Optimize matching zones to a given address #15275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bunnyshell Preview Environment deletedAvailable commands:
|
GSadee
previously approved these changes
Sep 8, 2023
jakubtobiasz
changed the title
jakubtobiasz
force-pushed
the
SYL-2705
branch
from
c77771e
to
6d55f85
Compare
jakubtobiasz
force-pushed
the
SYL-2705
branch
from
6d55f85
to
5a3923a
Compare
jakubtobiasz
changed the title
diimpp
reviewed
Sep 14, 2023
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Component/Addressing/spec/Matcher/ZoneMatcherSpec.php
Outdated
Show resolved
Hide resolved
jakubtobiasz
changed the title
jakubtobiasz
force-pushed
the
SYL-2705
branch
from
d942eb3
to
66a1b99
Compare
jakubtobiasz
force-pushed
the
SYL-2705
branch
from
66a1b99
to
7e975d3
Compare
jakubtobiasz
commented
Sep 15, 2023
There was a problem hiding this comment.
I have no idea how to make it working without the sqlite database file :|. If anyone has an idea – I encourage to share or directly contribute. Otherwise let's merge it and wait for new test apps.
jakubtobiasz
force-pushed
the
SYL-2705
branch
2 times, most recently
from
fabef59
to
2a47288
Compare
jakubtobiasz
force-pushed
the
SYL-2705
branch
from
2a47288
to
89a5e8a
Compare
NoResponseMate
previously approved these changes
Sep 22, 2023
diimpp
previously requested changes
Sep 24, 2023
| @@ -142,7 +143,7 @@ private function addResourcesSection(ArrayNodeDefinition $node): void | |||
| ->scalarNode('model')->defaultValue(Zone::class)->cannotBeEmpty()->end() | |||
| ->scalarNode('interface')->defaultValue(ZoneInterface::class)->cannotBeEmpty()->end() | |||
| ->scalarNode('controller')->defaultValue(ResourceController::class)->cannotBeEmpty()->end() | |||
| ->scalarNode('repository')->cannotBeEmpty()->end() | |||
| ->scalarNode('repository')->defaultValue(ZoneRepository::class)->cannotBeEmpty()->end() | |||
There was a problem hiding this comment.
Sylius bundles never specify repository at Configuration.php
I'm questioning this for years, but never actually research how and why it works this way.
There was a problem hiding this comment.
If there's a custom repository then it's declared in the config
There was a problem hiding this comment.
It's not true for 1.12, every instance that you've found was introduced in 1.13.
I still don't know why it was done this way and how it even work, but custom repositories are not specified for Configuration for any of the bundle
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/AddressingBundle/Repository/ZoneRepository.php
Outdated
Show resolved
Hide resolved
TheMilek
approved these changes
Sep 27, 2023
Rafikooo
approved these changes
Sep 27, 2023
Prometee
approved these changes
Sep 27, 2023
GSadee
approved these changes
Sep 27, 2023
|
Thanks, Jacob! 🎉 |
| /** | ||
| * @implements ZoneRepositoryInterface<ZoneInterface> | ||
| */ | ||
| class ZoneRepository extends EntityRepository implements ZoneRepositoryInterface |
There was a problem hiding this comment.
The fact that a new repository for an existing resource has shown up should be noted in the upgrade file. If someone has already created a custom one prior, the matcher will just fail.
| ]; | ||
|
|
||
| public function __construct(private RepositoryInterface $zoneRepository) | ||
| public function __construct(private ZoneRepositoryInterface $zoneRepository) |
There was a problem hiding this comment.
Should also be noted in the upgrade file
NoResponseMate
added a commit
that referenced
this pull request
Jan 17, 2024
This PR was merged into the 1.13 branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | 1.13 <!-- see the comment below --> | | Bug fix? | yes | | New feature? | no | | BC breaks? | not anymore | | Related tickets | #15275 (comment) | | License | MIT | Commits ------- [Upgrade] Note ZoneRepository addition and ZoneMatcher constructor changes [Addressing] Support BC for ZoneMatcher [ECS] Apply ecs fixes
GSadee
pushed a commit
to Sylius/SyliusAdminBundle
that referenced
this pull request
Jan 18, 2024
This PR was merged into the 1.13 branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | 1.13 <!-- see the comment below --> | | Bug fix? | yes | | New feature? | no | | BC breaks? | not anymore | | Related tickets | Sylius/Sylius#15275 (comment) | | License | MIT | Commits ------- [Upgrade] Note ZoneRepository addition and ZoneMatcher constructor changes [Addressing] Support BC for ZoneMatcher [ECS] Apply ecs fixes
This was referenced Oct 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test data:
~1000 zones <-> zone members 1:1
~20 zones created with up to 7 level of nesting (zone in the zone in the zone...)
Before:
After:
